Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post-Process example #5648

Merged
merged 60 commits into from
Feb 18, 2025
Merged

Post-Process example #5648

merged 60 commits into from
Feb 18, 2025

Conversation

enzofrancescaHM
Copy link
Contributor

Description:
Minimal example to show the possibility of implementing Post-Processing in A-Frame.
NOTE: for Post-Processing to work also in VR mode, supermedium/three.js#20 must be implemented in supermedium three.
Changes proposed:
-index.html running a simple scene with simple geometries with and without emission
-bloom.js, a minimal implementation of Bloom effect

@vincentfretin
Copy link
Contributor

Where does all those 1087 lines of code comes from?
You should be able to import the postprocessing library with an importmap, I advice you to base your example on the new importmap example https://github.com/aframevr/aframe/blob/master/examples/boilerplate/importmap/index.html and write a small bloom component of a few lines.
The bloom effect integration in r3f is just this https://github.com/pmndrs/react-postprocessing/blob/master/src/effects/Bloom.tsx

@enzofrancescaHM
Copy link
Contributor Author

Where does all those 1087 lines of code comes from? You should be able to import the postprocessing library with an importmap, I advice you to base your example on the new importmap example https://github.com/aframevr/aframe/blob/master/examples/boilerplate/importmap/index.html and write a small bloom component of a few lines. The bloom effect integration in r3f is just this https://github.com/pmndrs/react-postprocessing/blob/master/src/effects/Bloom.tsx

Yes, thanks for pointing that, I completely missed the new feature of A-Frame to use importmap. Sorry for that, I've reduced the bloom.js to the bare minimum to work.

@vincentfretin
Copy link
Contributor

Yes, thanks for pointing that, I completely missed the new feature of A-Frame to use importmap

That new importmap example exist since 2 days ago ;) I'm glad I did it so we can have a simpler example here without copying all the code.

@vincentfretin
Copy link
Contributor

Didn't we agree from supermedium/three.js#20 to use
https://github.com/pmndrs/postprocessing that is more performant than the three/addons effect composer?

@dmarcos
Copy link
Member

dmarcos commented Jan 30, 2025

Didn't we agree from supermedium/three.js#20 to use https://github.com/pmndrs/postprocessing that is more performant than the three/addons effect composer?

Whatever yields 90fps with the simpler code and least amount of dependencies

@enzofrancescaHM
Copy link
Contributor Author

Didn't we agree from supermedium/three.js#20 to use https://github.com/pmndrs/postprocessing that is more performant than the three/addons effect composer?

pmndrs does not work in VR at the moment. I have opened tickets there, but at the moment no one is working on it.

@dmarcos
Copy link
Member

dmarcos commented Jan 31, 2025

I merged THREE changes and updated A-Frame so should work on top of master.

Can you put the examples under showcase and rename to post-processing: showcase/post-processing?

Thanks so much for all the effort

Copy link
Contributor

@vincentfretin vincentfretin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the enabled prop in bloom component. You can use it in effect-controls

examples/showcase/post-processing/index.html Outdated Show resolved Hide resolved
examples/showcase/post-processing/bloom.js Outdated Show resolved Hide resolved
@dmarcos
Copy link
Member

dmarcos commented Feb 17, 2025

One more nit and ready to go:

#5648 (comment)

@@ -0,0 +1,36 @@
import AFRAME from 'aframe';
AFRAME.registerComponent('effect-controls', {
schema: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the file uses 4 spaces indentation instead of 2

@dmarcos
Copy link
Member

dmarcos commented Feb 18, 2025

effect-controls.js not passing the linter. needs adjustments. Comments in line:

https://github.com/aframevr/aframe/pull/5648/files#diff-5b6886b808dbe9941cd2151cf409ba3909afe015f4cec532df22e66a0b6ffd68

FYI. Linter should not let you commit if everything working as expected in your local setup

@enzofrancescaHM
Copy link
Contributor Author

yeah yeah, my bad, too many different projects with different setups. I usually run npm run lint:fix before committing, but for the 2 spaces indentation it is useless. Maybe we can add some prettier rules to the prettier rc like "prettier.tabWidth": 2, "prettier.useTabs": true,

@dmarcos
Copy link
Member

dmarcos commented Feb 18, 2025

yeah yeah, my bad, too many different projects with different setups. I usually run npm run lint:fix before committing, but for the 2 spaces indentation it is useless. Maybe we can add some prettier rules to the prettier rc like "prettier.tabWidth": 2, "prettier.useTabs": true,

@vincentfretin looks we don't have (lost?) indentation rules?

@dmarcos
Copy link
Member

dmarcos commented Feb 18, 2025

Thanks so much and for the patience. Usually first contributions don't get so involved 😄 Thanks for sticking to it and congrats

@dmarcos dmarcos merged commit 258de6c into aframevr:master Feb 18, 2025
3 checks passed
@enzofrancescaHM
Copy link
Contributor Author

Thanks so much and for the patience. Usually first contributions don't get so involved 😄 Thanks for sticking to it and congrats

Thanks to you and this wonderful community. I use A-Frame extensively, it seems to me fair to return something back to the community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants